feat(TokenEnterAmount): add new component to Swap flow #6247

sviderock · 2024-11-19T12:44:28Z

Final PR for new Enter Amount component. This page implements a 2nd version of SwapScreen that uses new Enter Amount component. It is hidden behind the feature flag to mitigate the risks of such a big rework.

SwapScreenV2.test.tsx file is an exact copy of the existing SwapScreen.test.tsx. It seems like a huge addition (2k lines) BUT it is a 99% copy-paste just because the new flow is implemented as a new component. The only changes in that file are testID props and some translations excerpts. To make life A LOT easier in reviewing this specific file, I suggest doing the following in VSCode to only see the changed lines:

Right click SwapScreen.test.tsx and click "Select for Compare"
Right click SwapScreenV2.test.tsx and click "Compare with Selected"

Details of the implementation for clarity:

The correct functioning of the new component has been ensured by implementing it to pass all the existing tests from SwapScreen.test.tsx.
Removed local Redux slice. As a new Enter Amount component is using useState and it would be a mess to keep multiple useStates and Redux slice in sync - all the other fields from the slice were moved to the corresponding useStates.
The business logic of the component is intact. Only the state implementation (useState instead of Redux slice) has changed to follow the existing logic therefore some functions look different but they do identical things in terms of business logic.
Code has been order in the following order:
1. variables
2. derived variables (useMemo)
3. useEffects
4. functions

Test plan

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-12-19.at.14.56.55.mp4

Related issues

Relates to RET-1208

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

…ter-amount-hook

bakoushin · 2024-12-23T16:09:07Z

src/swap/SwapScreenV2.tsx

+        keyboardShouldPersistTaps="handled"
+      >
+        <View style={{ display: 'flex', justifyContent: 'space-between', flexGrow: 1 }}>
+          <View style={{ flexShrink: 0 }}>


do we need this style?

@bakoushin it's indeed redundant, will remove!

bakoushin · 2024-12-23T16:21:42Z

src/swap/SwapScreenV2.tsx

+    }
+
+    if (fromToken?.tokenId === selectedToken.tokenId) {
+      setFromToken(toToken)


do we need to set "from" token in this handler?

please ignore, got it 😅

bakoushin · 2024-12-23T16:22:08Z

src/swap/SwapScreenV2.tsx

+    // if in "from" we select the same token as in "to" then just swap
+    if (toToken?.tokenId === selectedToken.tokenId) {
+      setFromToken(toToken)
+      setToToken(fromToken)


do we need to set "to" token in this handler?

please ignore, got it 😅

bakoushin · 2024-12-23T16:58:55Z

src/swap/SwapScreenV2.tsx

+    AppAnalytics.track(SwapEvents.swap_screen_select_token, { fieldType: Field.FROM })
+    // use requestAnimationFrame so that the bottom sheet open animation is done
+    // after the selectingField value is updated, so that the title of the
+    // bottom sheet (which depends on selectingField) does not change on the
+    // screen
+    requestAnimationFrame(() => tokenBottomSheetFromRef.current?.snapToIndex(0))


shall we consistenly use the hadndle prefix?

perhaps we can extract this code to a separate function e.g. handleOpenTokenPicker(fieldType: Field) to emphasize that hanlding is mostly identical

@bakoushin hard agree on both!

bakoushin · 2024-12-23T17:02:30Z

src/swap/SwapScreenV2.tsx

+
+        if (!fromToken || !toToken) {
+          // Should never happen
+          Logger.error(TAG, 'fromToken or toToken not found')


if that is a type guard (as "should never happen" comment suggests), do we need to log anything?

@bakoushin this log was present there from the start. I would keep it just in case somehow in the future we actually hit it for any miraculous reason (even though it should be impossible but who knows 🤷 )

bakoushin · 2024-12-23T17:04:03Z

src/swap/SwapScreenV2.tsx

+      // clearQuote,
+      // replaceAmountTo,


shall we remove it?

or uncomment? 😅

@bakoushin I'll add a TODO comment for this one. This useEffect initially lacked some dependencies which contradicts the rules of hooks but if I include all used variables/functions - it causes unnecessary re-runs which breaks a bunch of tests. So in order to make this optimization more clear - I'd prefer to make it in a separate PR, especially considering that currently the flow works as intended (just not following the rules of hooks).

So I've just left those commented there to not forget that they should also be acknowledged once I get to the point of optimizing it so I'll add a comment explaining that.

bakoushin · 2024-12-23T17:20:10Z

Epic work! 🎉

Left few nits/questions

emerge-tools · 2025-01-02T09:48:15Z

📸 Snapshot Test

No snapshots generated

Name	Added	Removed	Modified	Unchanged	Errored	Approval
Celo (test) org.celo.mobile.test	0	0	0	0	0	N/A

🛸 Powered by Emerge Tools

bakoushin · 2025-01-02T13:14:07Z

src/components/TokenEnterAmount.tsx

@@ -201,9 +215,11 @@ export function useEnterAmount(props: {
        displayAmount: getDisplayLocalAmount(parsedLocalAmount, localCurrencySymbol),
      },
    }
-  }, [amount, amountType, localCurrencySymbol])
+  }, [amount, amountType, localCurrencySymbol, props.token])


not directly related to this PR, but i'm trying to better understand the logic behind the inclusion/exclusion some variables to the dependency list.

for instance, we included the localCurrencySymbol, but not usdToLocalRate. yeah, it is highly unlikely that the rate could suddenly change while this component is rendered. but what is the reason for choosing one over another? it seems equally unlikely that the currency symbol will suddenly change.

@bakoushin that's just again my inattentiveness! Everything used in the scope should be included in the deps list. Will fix this now!

bakoushin · 2025-01-02T13:23:47Z

locales/base/translation.json

@@ -2823,5 +2823,6 @@
    "availableBalance": "Available: <0></0>",
    "selectToken": "Select token",
    "fiatPriceUnavailable": "Price unavailable"
-  }
+  },
+  "on": "on"


i guess the translation of the word "on" is highly dependent on context. perhaps we can use some template to provide it? kind of like this:

"tokenDescription": "{{tokenName}} on {{tokenNetwork}}"

and, with that, shall we include this item in the tokenEnterAmount group above?

do we need to keep the "on"?

bakoushin · 2025-01-02T13:57:44Z

src/earn/poolInfoScreen/BeforeDepositBottomSheet.tsx

@@ -283,6 +285,9 @@ export default function BeforeDepositBottomSheet({
  const { t } = useTranslation()

  const { availableShortcutIds } = pool


not sure if those files should appear in the diff

@bakoushin oops, that's a merge going wrong.

@bakoushin this one's resolved!

bakoushin · 2025-01-02T14:08:38Z

src/points/PointsHistoryBottomSheet.test.tsx

@@ -127,7 +127,7 @@ describe(PointsHistoryBottomSheet, () => {
    ).toBeTruthy()
    expect(tree.getByText('points.history.cards.createWallet.subtitle')).toBeTruthy()

-    expect(tree.getByText('March')).toBeTruthy()
+    expect(tree.getByText('March', { exact: false })).toBeTruthy()


perhaps this change is worth its own (yet tiny) PR, so other team members could benefit from the fix before this epic PR is merged.
also, it will provide more context as to why it is fixed.

@bakoushin addressed in this PR: #6392

…ion string

bakoushin · 2025-01-02T14:47:18Z

src/swap/SwapScreenV2.tsx

+        [Field.FROM]: processedAmountsFrom.token.bignum
+          ? processedAmountsFrom.token.bignum.toString()
+          : '',


supernit: perhaps we can make use of a nullish coalescing operator and save 2 lines here? 😅

processedAmountsFrom.token.bignum?.toString() ?? ''

@bakoushin it's even gonna be 4 lines cause it has to be done for both FROM and TO 😄

bakoushin · 2025-01-02T14:51:18Z

src/swap/SwapScreenV2.tsx

+    let newSwitchedToNetwork: typeof switchedToNetworkId | null = null
+
+    switch (true) {
+      // If were selecting a field that was already selected in the other input then switch inputs


looks like typo: "we're"

bakoushin · 2025-01-02T15:19:42Z

src/swap/SwapScreenV2.tsx

+    if (!fromToken) {
+      // Should never happen
+      return
+    }
+    AppAnalytics.track(SwapEvents.swap_screen_percentage_selected, {
+      tokenSymbol: fromToken.symbol,
+      tokenId: fromToken.tokenId,
+      tokenNetworkId: fromToken.networkId,
+      percentage,
+    })
+  }
+
+  function onPressLearnMore() {
+    AppAnalytics.track(SwapEvents.swap_learn_more)
+    navigate(Screens.WebViewScreen, { uri: links.swapLearnMore })
+  }
+
+  function onPressLearnMoreFees() {
+    AppAnalytics.track(SwapEvents.swap_gas_fees_learn_more)
+    navigate(Screens.WebViewScreen, { uri: links.transactionFeesLearnMore })
+  }
+
+  return (
+    <SafeAreaView style={styles.safeAreaContainer} testID="SwapScreen">
+      <CustomHeader


stylistic nit: most of the handlers above start with handle-, but these two start with on-. it would be nice to have a consistent naming (my preference is to keep using handle-)

@bakoushin fixed!

bakoushin

🚀

sviderock added 18 commits November 18, 2024 16:44

add new TokenEnterAmount component

e5ab0a5

fix exports

2fbc1b2

Merge branch 'main' into slava/add-new-enter-amount-component

b12d50f

Add reusable useEnterAmount hook

8af7b8d

remove unused code

44c0aab

change flow on send

1aa17d6

fix tests

4ed89fc

fix jumpstart

d0c7099

change earn flow

98d6552

remove unnecessary changes

b7e5107

Merge branch 'main' into slava/add-new-enter-amount-component

6ca6f92

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

f2185bc

…ter-amount-hook

Merge branch 'slava/add-enter-amount-hook' into slava/change-send-flow

8762401

fix knip

98938bc

uncomment some tests

5a3f776

fixes

9360626

Merge branch 'slava/change-send-flow' into slava/change-earn-flow

f322e23

wip: change swap flow

a54d71f

sviderock changed the base branch from main to slava/change-earn-flow November 19, 2024 12:44

sviderock added 11 commits November 20, 2024 15:39

add tests

b3f3c67

clear up console logs

8ce68a4

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

c2773fa

…ter-amount-hook

WIP: tests for the hook

37455cb

reformat groupNumber, finish number formatting tests

8357b18

add useEnterAmount initial tests

f5db952

finish tests

130b620

revert unnecessary change

10fb5bd

Merge branch 'main' into slava/add-new-enter-amount-component

112e5f9

Merge branch 'slava/add-new-enter-amount-component' into slava/add-en…

bce9c2b

…ter-amount-hook

Merge branch 'slava/add-enter-amount-hook' into slava/change-send-flow

7048f8e

bakoushin reviewed Dec 23, 2024

View reviewed changes

multiple fixes as per review

726153d

bakoushin reviewed Jan 2, 2025

View reviewed changes

Merge branch 'main' into slava/change-swap-flow

9c6886d

sviderock force-pushed the slava/change-swap-flow branch from 0130a21 to 9c6886d Compare January 2, 2025 14:21

sviderock added 3 commits January 2, 2025 16:22

fix incomplete list of deps for processedAmounts memo

27a7f58

more fixes as per review

67c69ba

fix tests affected by moving token description to a separate translat…

68319ed

…ion string

bakoushin reviewed Jan 2, 2025

View reviewed changes

sviderock added 2 commits January 2, 2025 17:00

more fixes as per review

bd8999c

fix typo

e6b5971

bakoushin reviewed Jan 2, 2025

View reviewed changes

change remaining "on" handlers with "handle"

7caeb77

bakoushin approved these changes Jan 2, 2025

View reviewed changes

Merge branch 'main' into slava/change-swap-flow

daa64a9

sviderock added this pull request to the merge queue Jan 6, 2025

Merged via the queue into main with commit 9815138 Jan 6, 2025
16 checks passed

sviderock deleted the slava/change-swap-flow branch January 6, 2025 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(TokenEnterAmount): add new component to Swap flow #6247

feat(TokenEnterAmount): add new component to Swap flow #6247

sviderock commented Nov 19, 2024 •

edited

Loading

bakoushin Dec 23, 2024

sviderock Jan 2, 2025

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

sviderock Jan 2, 2025

bakoushin Dec 23, 2024 •

edited

Loading

sviderock Jan 2, 2025

bakoushin Dec 23, 2024

bakoushin Dec 23, 2024

sviderock Jan 2, 2025

bakoushin commented Dec 23, 2024

emerge-tools bot commented Jan 2, 2025 •

edited

Loading

bakoushin Jan 2, 2025

sviderock Jan 2, 2025

bakoushin Jan 2, 2025

bakoushin Jan 2, 2025

bakoushin Jan 2, 2025

sviderock Jan 2, 2025

sviderock Jan 2, 2025

bakoushin Jan 2, 2025

sviderock Jan 2, 2025 •

edited

Loading

bakoushin Jan 2, 2025

sviderock Jan 2, 2025 •

edited

Loading

bakoushin Jan 2, 2025

bakoushin Jan 2, 2025

sviderock Jan 2, 2025

bakoushin left a comment

		@@ -283,6 +285,9 @@ export default function BeforeDepositBottomSheet({
		const { t } = useTranslation()

		const { availableShortcutIds } = pool

feat(TokenEnterAmount): add new component to Swap flow #6247

feat(TokenEnterAmount): add new component to Swap flow #6247

Conversation

sviderock commented Nov 19, 2024 • edited Loading

Test plan

Related issues

Backwards compatibility

Network scalability

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin commented Dec 23, 2024

emerge-tools bot commented Jan 2, 2025 • edited Loading

📸 Snapshot Test

No snapshots generated

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakoushin left a comment

Choose a reason for hiding this comment

sviderock commented Nov 19, 2024 •

edited

Loading

bakoushin Dec 23, 2024 •

edited

Loading

emerge-tools bot commented Jan 2, 2025 •

edited

Loading

sviderock Jan 2, 2025 •

edited

Loading

sviderock Jan 2, 2025 •

edited

Loading